Skip to content

Conversation

@wendigo
Copy link
Contributor

@wendigo wendigo commented Nov 28, 2025

Description

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Nov 28, 2025
@wendigo wendigo requested a review from findepi November 28, 2025 20:55
@wendigo wendigo force-pushed the serafin/scoped-value branch from 51fcd56 to b5a6252 Compare November 29, 2025 12:21
@wendigo wendigo force-pushed the serafin/scoped-value branch from b5a6252 to 4cdc446 Compare November 29, 2025 12:28
@wendigo wendigo requested a review from losipiuk December 1, 2025 10:02
@wendigo wendigo merged commit 56e1e16 into trinodb:master Dec 2, 2025
63 checks passed
@github-actions github-actions bot added this to the 479 milestone Dec 2, 2025
@findepi
Copy link
Member

findepi commented Dec 2, 2025

ThreadLocals (TL) distinguish between inheritable (InheritableThreadLocal) and non-inheritable (the default). Only the non-inheritable TLs behave reasonably, especially when thread pools are involved. The inheritable TLs definitely have some safe application patterns, but I am yet to learn them.

ScopedValue are a much more convenient API than ThreadLocal. (ScopedValue<T> is like ThreadLocal<Stack<T>> with nice API).
However, they are not a safe replacement for TL. This is because ScopedValue are always inheritable (via StructuredTaskScope).

Here is a program demonstrating that ReentrantBoundedExecutor using ScopedValue no longer enforces its bounds.

static void main()
{
    ReentrantBoundedExecutor executor = new ReentrantBoundedExecutor(
            Executors.newCachedThreadPool(),
            1);

    executor.execute(() -> {
        // This looks rogue, but that can be some other somewhere else using StructuredTaskScope to transport some other ScopedValue to some other place
        try (var scope = StructuredTaskScope.open()) {
            CyclicBarrier barrier = new CyclicBarrier(2);
            var first = scope.fork(() -> {
                executor.execute(() -> {
                    System.out.println("currentThread().getName() = " + currentThread().getName());
                    try {
                        barrier.await();
                    }
                    catch (Exception e) {
                        throw new RuntimeException(e);
                    }
                    System.out.println("two threads are in");
                });
            });
            var second = scope.fork(() -> {
                executor.execute(() -> {
                    System.out.println("currentThread().getName() = " + currentThread().getName());
                    try {
                        barrier.await();
                    }
                    catch (Exception e) {
                        throw new RuntimeException(e);
                    }
                    System.out.println("two threads are in");
                });
            });
            scope.join();
        }
        catch (Exception e) {
            throw new RuntimeException(e);
        }
    });
}

implements Executor
{
private final ThreadLocal<Boolean> executorThreadMarkers = ThreadLocal.withInitial(() -> false);
private final ScopedValue<Void> executorThreadMarkers = ScopedValue.newInstance();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just got to know the usage of it 👍

@findepi
Copy link
Member

findepi commented Dec 3, 2025

Here is a program demonstrating that ReentrantBoundedExecutor using ScopedValue no longer enforces its bounds.

To be honest, ReentrantBoundedExecutor is pretty weak about enforcing its bounds -- that's the difference between ReentrantBoundedExecutor and regular BoundedExecutor. A task executing inside ReentrantBoundedExecutor has a free pass to insert arbitrary number of additional tasks into the executor. This was true before the changes and is true after. What this changed, is that there is another avenue for inserting arbitrary number of additional tasks -- via the StructuredTaskScope (preview API in 25).

@wendigo wendigo deleted the serafin/scoped-value branch December 3, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants